Rsync and Fuse Issues #139

mtelvers · 2023-01-15T14:24:55Z

The rsync folder result-tmp grows uncontrollably, eventually leading to the worker stalling on with the following error:

2023-01-15 13:22.58       obuilder [INFO] Pruning 0 items (of 100 requested)
2023-01-15 13:22.58         worker [INFO] OBuilder partition: 7% free after pruning 0 items
2023-01-15 13:22.58         worker [WARNING] Out of space, but nothing left to prune! (will wait and then retry)

This is caused by the following two considitons:

when a job is cancelled, the macOS sandbox does not delete the result-tmp folder, and
when a job fails, the rsync backend does not delete the result-tmp.

Furthermore, under normal circumstances, the fuse file system is unmounted between jobs. This is essential for the Docker health check to be able to find the Docker binary which is installed in /usr/local. However, when a job is cancelled, fuse is not unmounted; therefore, we see the health check fail if it occurs immediately after a cancelled job.

MisterDA

LGTM. Could you squash the commits when you'll think this is ready for merging?

lib/rsync_store.ml

lib/sandbox.macos.ml

tmcgilchrist

Some minor style suggestions but otherwise the change looks good.
We need an osx rsync stress test to validate we don't leak disk space.

MisterDA · 2023-01-17T23:44:54Z

lib/sandbox.macos.ml

-      if Lwt.is_sleeping proc then (
-        match !proc_id with
+      if Lwt.is_sleeping proc then
+        let _ = match !proc_id with


I don't understand exactly what's happening here, but it looks wrong as you're not binding the Lwt promise, or am I reading that wrong?

I have updated the binding and changed the function a bit as well.

tmcgilchrist · 2023-01-18T21:57:14Z

The LWT sleep looks curious to me. Does the kill_all_descendants function not wait for the command to terminate?

…

On Wed, 18 Jan 2023 at 6:35 pm, Mark Elvers ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In lib/sandbox.macos.ml <#139 (comment)>: > @@ -127,12 +127,11 @@ let run ~cancelled ?stdin:stdin ~log (t : t) config result_tmp = in Lwt.on_termination cancelled (fun () -> let aux () = - post_cancellation ~result_tmp t >>= fun () -> - if Lwt.is_sleeping proc then ( - match !proc_id with + if Lwt.is_sleeping proc then + let _ = match !proc_id with I have updated the binding and changed the function a bit as well. — Reply to this email directly, view it on GitHub <#139 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABJXOPLD6BHOXR3QHVXLZTWS6MNRANCNFSM6AAAAAAT34STOY> . You are receiving this because you commented.Message ID: ***@***.***>

mtelvers · 2023-01-18T22:29:34Z

kill_all_descendants invokes multiple kill -9 pid commands. The sleep was an attempt to allow the processes time to actually terminate and be cleared up by the operating system before trying to delete any files they had open.

MisterDA · 2023-01-23T12:36:31Z

The LWT sleep looks curious to me. Does the kill_all_descendants function not wait for the command to terminate?

kill_all_descendants invokes multiple kill -9 pid commands. The sleep was an attempt to allow the processes time to actually terminate and be cleared up by the operating system before trying to delete any files they had open.

I've looked in depth, and I'm convinced that we're correctly waiting until all kill have finished, but maybe macOS does still require some time to catch up.

MisterDA · 2023-01-23T12:46:17Z

lib/sandbox.macos.ml

-          )
-      else Lwt.return_unit  (* Process has already finished *)
+      else Lwt.return_unit >>= fun () ->
+      post_cancellation ~result_tmp t


What I meant is that the indentation of post_cancellation is confusing: it is actually part of the else block here. If you want to execute it regardless of the Lwt.is_sleeping proc, you have to enclose the if … else … block in a begin … end (or parenthesis) block.
I think that's what you want.

open Lwt.Infix let () = let x = ref true in Lwt_main.run begin if !x then Lwt_io.print "hello" else Lwt_io.print "world" >>= fun () -> Lwt_io.print "else branch" end (* prints "hello" *) let () = let x = ref true in Lwt_main.run begin begin if !x then Lwt_io.print "hello" else Lwt_io.print "world" end >>= fun () -> Lwt_io.print " common" end (* prints "hello common" *)

You could configure your editor to use ocp-indent (since we're not applying ocamlformat here).

Yes, I have an updated version with that modification which I haven't pushed yet.

MisterDA · 2023-01-23T12:51:13Z

lib/sandbox.macos.ml

    post_build ~result_dir ~home_dir t >>= fun () ->
    Lwt.return r
  in
+  let () = Lwt.on_failure cancelled (fun _ -> let _ = unmount_fuse t in () )


The returned promise most likely needs to be passed to Lwt.async to be executed by Lwt, just like the function passed to Lwt.on_termination below.

I think that on_failure can be removed as on_termination covers both cases.

mtelvers · 2023-01-23T12:52:16Z

Thanks @MisterDA. Right now, the issue I am seeing is that when a job is cancelled, a new job is started immediately, but new job appears to run concurrently with the on_termination code. For a normal worker, this isn't a problem because jobs can run concurrently, but on the Mac, the new job is starting as the old one is still finishing both using the same home directory.

MisterDA · 2023-01-31T16:31:33Z

What's the state of this PR now? Are you satisfied with your changes?

MisterDA · 2023-02-09T16:49:08Z

Merged as of 66f5164, thanks!

- Updates to address rsync and sandbox issues (ocurrent/obuilder#139) - Minor updates.

@mtelvers

CHANGES: - Updates to address rsync and sandbox issues. (@mtelvers ocurrent/obuilder#139, reviewed by @tmcgilchrist and @MisterDA) - Add an obuilder clean command to clean all build results. (@MisterDA ocurrent/obuilder#140, reviewed by @tmcgilchrist) - Make rsync-mode mandatory when using rsync store. (@tmcgilchrist ocurrent/obuilder#132, reviewed by @kit-ty-kate and @MisterDA)

- Updates to address rsync and sandbox issues (ocurrent/obuilder#139) - Minor updates.

MisterDA approved these changes Jan 16, 2023

View reviewed changes

lib/rsync_store.ml Outdated Show resolved Hide resolved

tmcgilchrist reviewed Jan 16, 2023

View reviewed changes

lib/rsync_store.ml Show resolved Hide resolved

tmcgilchrist reviewed Jan 16, 2023

View reviewed changes

lib/sandbox.macos.ml Outdated Show resolved Hide resolved

tmcgilchrist approved these changes Jan 16, 2023

View reviewed changes

MisterDA reviewed Jan 17, 2023

View reviewed changes

MisterDA reviewed Jan 23, 2023

View reviewed changes

MisterDA requested changes Jan 23, 2023

View reviewed changes

Updates to address rsync and sandbox issues

fc2a064

mtelvers force-pushed the delete-debug branch from ae9cdaa to fc2a064 Compare January 23, 2023 18:24

MisterDA force-pushed the master branch 2 times, most recently from 89e84ac to ccd3f2f Compare February 3, 2023 12:39

MisterDA added a commit that referenced this pull request Feb 9, 2023

Merge pull request #139 from mtelvers:delete-debug

739b1a3

MisterDA added a commit that referenced this pull request Feb 9, 2023

Merge pull request #139 from mtelvers/delete-debug

66f5164

MisterDA closed this Feb 9, 2023

MisterDA added a commit to ocurrent/ocluster that referenced this pull request Feb 10, 2023

Update OBuilder

91a628d

- Updates to address rsync and sandbox issues (ocurrent/obuilder#139) - Minor updates.

MisterDA added a commit to ocurrent/ocluster that referenced this pull request Feb 10, 2023

Update OBuilder

4c3ecf5

- Updates to address rsync and sandbox issues (ocurrent/obuilder#139) - Minor updates.

MisterDA mentioned this pull request Feb 17, 2023

[new release] obuilder and obuilder-spec (0.5.1) ocaml/opam-repository#23346

Merged

benmandrew pushed a commit to benmandrew/ocluster that referenced this pull request Mar 6, 2023

Update OBuilder

ad92254

- Updates to address rsync and sandbox issues (ocurrent/obuilder#139) - Minor updates.

mtelvers mentioned this pull request Mar 10, 2023

Docker health check occasionally fails on Mac Workers ocurrent/macos-infra#20

Closed

Rsync and Fuse Issues #139

Rsync and Fuse Issues #139

Uh oh!

Conversation

mtelvers commented Jan 15, 2023

Uh oh!

MisterDA left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tmcgilchrist left a comment

Choose a reason for hiding this comment

Uh oh!

MisterDA Jan 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtelvers Jan 18, 2023

Choose a reason for hiding this comment

Uh oh!

tmcgilchrist commented Jan 18, 2023 via email

Uh oh!

mtelvers commented Jan 18, 2023

Uh oh!

MisterDA commented Jan 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MisterDA Jan 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtelvers Jan 23, 2023

Choose a reason for hiding this comment

Uh oh!

MisterDA Jan 23, 2023

Choose a reason for hiding this comment

Uh oh!

mtelvers Jan 23, 2023

Choose a reason for hiding this comment

Uh oh!

mtelvers commented Jan 23, 2023

Uh oh!

MisterDA commented Jan 31, 2023

Uh oh!

MisterDA commented Feb 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MisterDA Jan 17, 2023 •

edited

Loading

MisterDA commented Jan 23, 2023 •

edited

Loading

MisterDA Jan 23, 2023 •

edited

Loading